Reopen profile picker on reselect + WebDAV icons#42
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughIntroduces ChangesOutboundPreference reselection and VALUE_SELECT_PROFILE refactor
WebDAV preference icons
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adopts two upstream (starifly) commits, cherry-picked cleanly onto our fork:
196257c) — fixes a real UX bug in the outbound / front-proxy / landing-proxy / route-rule dropdowns. When the current value is already "Select profile", Android'sSpinnerdoes not fireonItemSelectedon re-tap, so the profile chooser couldn't be reopened to pick a different profile without first switching to another option and back. Adds aReselectableSpinner(fires the selection callback on re-select), pre-selects the current profile inProfileSelectActivityviaEXTRA_SELECTED, and replaces the magic string"3"with a namedOutboundPreference.VALUE_SELECT_PROFILEconstant.18b4bc5) — cosmetic: adds row icons to the WebDAV settings screen (one new drawableic_baseline_folder_open_24; the other four icons already existed).Notes
main(no conflicts).ProfileSelectActivity.EXTRA_SELECTEDalready exists and is handled; the new reselectable dropdown layout mirrors androidx'spreference_dropdown_material.xml.ReselectableSpinner.onWindowFocusChangeddocumenting that window-focus is a best-effort proxy for popup dismissal (may fire on app foregrounding, IME, system dialogs, rotation); the reselect logic stays correct if it fires spuriously.Testing
assembleOssDebugbuilds clean; CodeRabbit local review: 0 findings.Not for merge yet — pending on-device verification of the reselect behavior.
Greptile Summary
This PR cherry-picks two upstream commits: a UX fix that allows the "Select Profile" spinner to be reopened when the current value is already "Select Profile" (by introducing
ReselectableSpinnerand wiring it intoOutboundPreference), and a cosmetic change that adds row icons to the WebDAV settings screen.ReselectableSpinneroverrides bothsetSelectionoverloads to re-fireonItemSelectedwhen the same position is selected.OutboundPreferencegains adropdownOpenedflag (set inonClick, cleared after each selection or popup close) and aselectionReadypost-guard to distinguish initial binding from live interaction.GroupSettingsActivityandRouteSettingsActivityare updated to pass the currently-selected profile asEXTRA_SELECTEDso the picker pre-selects the right row on reopen.app:iconattributes added towebdav_preferences.xml; onlyic_baseline_folder_open_24is new."3"for the "select profile" spinner index is replaced byOutboundPreference.VALUE_SELECT_PROFILEthroughout.Confidence Score: 4/5
Safe to merge after on-device verification of the reselect path; the WebDAV icon change is entirely cosmetic and risk-free.
The core logic is sound and well-guarded. The two things worth watching are: (1) ReselectableSpinner.setSelection(Int) delegates to super which bounces back through setSelection(Int, Boolean) via virtual dispatch, so notifyReselected fires twice per same-position selection — currently harmless because the first fire clears dropdownOpened, but it means callers receive onItemSelected twice for one gesture; and (2) a narrow race where a notification banner or IME event lands between the user opening the dropdown and re-tapping 'Select Profile' will silently swallow that tap. Both are documented limitations, not unknown gaps. No data-loss or crash risk is introduced.
ReselectableSpinner.kt warrants a second look on the double-notification path and the window-focus proxy behaviour before this is promoted to a release build.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[User taps Spinner row] --> B[OutboundPreference.onClick\ndropdownOpened = true] B --> C[Dropdown opens] C --> D{User selects item} D --> |different item| E[Spinner fires onItemSelected\nnewValue != value] D --> |same SELECT_PROFILE item| F[ReselectableSpinner.setSelection\nreselected = true] F --> G[notifyReselected fires\nonItemSelected callback] E --> H[OutboundPreference.onItemSelected\nselectionReady check] G --> H H --> I{reselectedProfile?\ndropdownOpened &&\nnewValue == value &&\nnewValue == VALUE_SELECT_PROFILE} I --> |yes - reselect| J[callChangeListener] I --> |no - new value| K{newValue != value?} K --> |yes| J K --> |no| L[no-op] J --> M[PreferenceChangeListener\nin Activity] M --> N{value == VALUE_SELECT_PROFILE?} N --> |yes| O[Launch ProfileSelectActivity\nwith EXTRA_SELECTED = current profile] N --> |no| P[Update preference value\nreturn true] O --> Q[dropdownOpened = false] P --> Q Q --> R[onWindowFocusChanged fires\nwhen popup closes\nonPopupClosed → dropdownOpened = false]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[User taps Spinner row] --> B[OutboundPreference.onClick\ndropdownOpened = true] B --> C[Dropdown opens] C --> D{User selects item} D --> |different item| E[Spinner fires onItemSelected\nnewValue != value] D --> |same SELECT_PROFILE item| F[ReselectableSpinner.setSelection\nreselected = true] F --> G[notifyReselected fires\nonItemSelected callback] E --> H[OutboundPreference.onItemSelected\nselectionReady check] G --> H H --> I{reselectedProfile?\ndropdownOpened &&\nnewValue == value &&\nnewValue == VALUE_SELECT_PROFILE} I --> |yes - reselect| J[callChangeListener] I --> |no - new value| K{newValue != value?} K --> |yes| J K --> |no| L[no-op] J --> M[PreferenceChangeListener\nin Activity] M --> N{value == VALUE_SELECT_PROFILE?} N --> |yes| O[Launch ProfileSelectActivity\nwith EXTRA_SELECTED = current profile] N --> |no| P[Update preference value\nreturn true] O --> Q[dropdownOpened = false] P --> Q Q --> R[onWindowFocusChanged fires\nwhen popup closes\nonPopupClosed → dropdownOpened = false]Reviews (1): Last reviewed commit: "fix: reopen profile picker on reselect" | Re-trigger Greptile